Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update SmartSim to use Dragon V0.10 #753

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

Update SmartSim to use Dragon V0.10 #753

wants to merge 16 commits into from

Conversation

MattToast
Copy link
Member

@MattToast MattToast commented Oct 17, 2024 •
edited
Loading

Update SmartSim to use Dragon V0.10 by:

  • Removing the use of Dragon attributes that have been removed in Dragon V0.10.
  • Updating smart build --dragon ... to fetch Dragon V0.10 once all required assets are hosted
  • Run through full on WLM test suite without errors

@MattToast MattToast added type: refactor Issues focused on refactoring existing code area: launcher Issues related to any of the launchers within SmartSim bug: major A major bug area: Dragon labels Oct 17, 2024
@MattToast MattToast self-assigned this Oct 17, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 18, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 35.84906% with 34 lines in your changes missing coverage. Please review.

Project coverage is 81.82%. Comparing base (d7d979e) to head (eb7639b).
Report is 16 commits behind head on develop.

Files with missing lines Patch % Lines
smartsim/_core/launcher/dragon/dragonBackend.py 27.65% 34 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #753      +/-   ##
===========================================
- Coverage    83.91%   81.82%   -2.09%     
===========================================
  Files           83       84       +1     
  Lines         6284     7081     +797     
===========================================
+ Hits          5273     5794     +521     
- Misses        1011     1287     +276
Files with missing lines Coverage Δ
smartsim/_core/utils/__init__.py 100.00% <ø> (ø)
smartsim/_core/utils/helpers.py 91.58% <100.00%> (+0.12%) ⬆️
smartsim/_core/launcher/dragon/dragonBackend.py 35.35% <27.65%> (-0.11%) ⬇️

... and 46 files with indirect coverage changes

@MattToast MattToast marked this pull request as ready for review October 18, 2024 21:16
al-rigazzi
al-rigazzi approved these changes Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

smartsim/_core/launcher/dragon/dragonBackend.py Outdated Show resolved Hide resolved
@al-rigazzi
Copy link
Collaborator

@MattToast there have been some updates to Dragon's ProcessGroup management which require some modification of this code. Do you want me to take care of that by pushing to your branch?

@al-rigazzi al-rigazzi self-requested a review December 9, 2024 23:06
al-rigazzi
al-rigazzi requested changes Dec 9, 2024
View reviewed changes
Copy link
Collaborator

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor changes need to happen before this can be merged. They include:

  1. change pinned version
  2. add arg to ProcessGroup to avoid failing on process exception
  3. handle exceptions when stopping groups and redir workers

@MattToast
Copy link
Member Author

@MattToast there have been some updates to Dragon's ProcessGroup management which require some modification of this code. Do you want me to take care of that by pushing to your branch?

@al-rigazzi By all means! Feel free to take over ownership of this branch and push whatever changes are necessary!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ankona ankona ankona left review comments

@al-rigazzi al-rigazzi al-rigazzi requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@al-rigazzi al-rigazzi

@MattToast MattToast

Labels
area: Dragon area: launcher Issues related to any of the launchers within SmartSim bug: major A major bug type: refactor Issues focused on refactoring existing code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MattToast @al-rigazzi @ankona